-
Notifications
You must be signed in to change notification settings - Fork 205
feat: Add new resource mongodbatlas_cloud_user_project_assignment #3568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: CLOUDP-320243-dev-2.0.0
Are you sure you want to change the base?
Conversation
This reverts commit f8d2d0a.
…m-provider-mongodbatlas into CLOUDP-320243-dev-2.0.0
…m-provider-mongodbatlas into CLOUDP-320243-dev-2.0.0
APIx bot: a message has been sent to Docs Slack channel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new Terraform resource mongodbatlas_cloud_user_project_assignment
for managing user assignments to MongoDB Atlas projects with specific roles.
Key changes:
- Implements a new resource with full CRUD operations and import functionality
- Adds comprehensive test coverage including basic functionality and migration scenarios
- Integrates the resource into the provider registration and CI/CD workflows
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
internal/service/clouduserprojectassignment/resource.go | Core resource implementation with CRUD operations and import logic |
internal/service/clouduserprojectassignment/resource_schema.go | Generated schema definition and TFModel struct |
internal/service/clouduserprojectassignment/model.go | Data transformation functions between Terraform and Atlas SDK models |
internal/service/clouduserprojectassignment/resource_test.go | Basic acceptance tests with create, update, and import scenarios |
internal/service/clouduserprojectassignment/resource_migration_test.go | Migration tests for transitioning from project invitation to user assignment |
internal/service/clouduserprojectassignment/model_test.go | Unit tests for model transformation functions |
internal/service/clouduserprojectassignment/main_test.go | Test setup and teardown |
internal/service/clouduserprojectassignment/tfplugingen/generator_config.yml | Configuration for code generation |
internal/provider/provider.go | Registration of the new resource in the provider |
.github/workflows/acceptance-tests-runner.yml | CI configuration updates for the new service |
.changelog/3568.txt | Release note for the new resource |
resp.Diagnostics.Append(resp.State.Set(ctx, newCloudUserProjectAssignmentModel)...) | ||
} | ||
|
||
func fetchProjectUser(ctx context.Context, connV2 *admin.APIClient, projectID, userID, username string) (*admin.GroupUserResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetchProjectUser
function is introduced here and reused in #3569 to extract common functionality between the resource and the data source.
} | ||
|
||
for _, removeReq := range removeRequests { | ||
_, _, err := connV2.MongoDBCloudUsersApi.RemoveProjectRole(ctx, projectID, userID, removeReq).Execute() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not blocking - if this API fails, the update will be partially applied (only the addprojectRole). This should be documented at minimum, but also we should check if we're consistent in similar situations
cc @maastha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by documented, do you mean add a comment here or mention something in the public documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should publicly document it? because I can see a question coming, like: "the update failed but then I logged in and saw some users were added ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but after failure when they run terraform plan again they will see the exact status of what roles were added vs which were not so I think that should suffice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, you have a fair point with this one. I'd say an extra clarification in the doc doesn't hurt, especially in case of customers reaching out. But it's a nice to have.
plan *clouduserprojectassignment.TFModel | ||
expected *admin.GroupUserRequest | ||
}{ | ||
"Complete model": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also add test cases for empty/nil values (for all unit tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 59950ba
return &addProjectUserReq, nil | ||
} | ||
|
||
func NewAtlasUpdateReq(ctx context.Context, plan, state *TFModel) (addRequests, removeRequests []*admin.AddOrRemoveGroupRole, diags diag.Diagnostics) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking if instead of getting roles from the state here, would it be better to compare against what is returned by the API instead? That ways any partially applied calls can be handled correctly WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean making another API call before calling this method in the Update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
resource.ParallelTest(t, *basicTestCase(t)) | ||
} | ||
|
||
func basicTestCase(t *testing.T) *resource.TestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also have a test case to import an existing (ACTIVE) project user? is that possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@csanx are there any docs updates for this feature? If not, LGTM
I see https://github.com/mongodb/terraform-provider-mongodbatlas/pull/3578/files so I'll review that and approve this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, leaving some doubts which seem more API related
importID := req.ID | ||
ok, parts := conversion.ImportSplit(req.ID, 2) | ||
if !ok { | ||
resp.Diagnostics.AddError("invalid import ID format", "expected 'project_id/user_id' or 'project_id/username', got: "+importID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: Why do we support import with user_id
as well? Does it help with a particular migration journey? Asking as this does add some complexity to our read operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s mainly for consistency, since we provide both keys in all Cloud User resources
internal/service/clouduserprojectassignment/resource_migration_test.go
Outdated
Show resolved
Hide resolved
resource "mongodbatlas_project_invitation" "mig_test" { | ||
project_id = mongodbatlas_project.mig_test.id | ||
username = local.username | ||
roles = local.roles | ||
} | ||
|
||
resource "mongodbatlas_cloud_user_project_assignment" "user_mig_test" { | ||
project_id = mongodbatlas_project.mig_test.id | ||
username = local.username | ||
roles = local.roles | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can user_project_assignment
be created with project_invitation
already existing? I suppose this is how the API behaves but wanted to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, after running this, the state shows:
{
"mode": "managed",
"type": "mongodbatlas_project_invitation",
"name": "mig_test",
...
},
{
"mode": "managed",
"type": "mongodbatlas_cloud_user_project_assignment",
"name": "user_mig_test",
...
},
We can also see in Cloud Dev that the same user is invited twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks. This sentence for the Tech design helped me understand this migration journey, potentially we can leave it as a comment:
we only need to consider the migration path for mongodbatlas_project_invitation resources with a PENDING invite (similar to org_invites) - which are not discoverable by the new APIs - so the only way to migrate here is to re-create the pending users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I’ll take that into account
@jwilliams-mongo Yes, my upcoming PRs will add documentation for all new resources and data sources. The one you mention is related to another resource actually |
@@ -601,8 +602,10 @@ jobs: | |||
MONGODB_ATLAS_PROJECT_OWNER_ID: ${{ inputs.mongodb_atlas_project_owner_id }} | |||
MONGODB_ATLAS_ORG_ID: ${{ inputs.mongodb_atlas_org_id }} | |||
MONGODB_ATLAS_USERNAME: ${{ vars.MONGODB_ATLAS_USERNAME }} | |||
MONGODB_ATLAS_USERNAME_2: ${{ vars.MONGODB_ATLAS_USERNAME_2 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any difference between MONGODB_ATLAS_USERNAME & MONGODB_ATLAS_USERNAME_2? or we're just adding for more test cases? thinking if we should add a comment here on what MONGODB_ATLAS_USERNAME_2 signifies
Description
Add new resource:
mongodbatlas_cloud_user_project_assignment
Link to any related issue(s): CLOUDP-333176
Type of change:
Required Checklist:
Further comments